-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dbeaver/pro#2795 Editors for objects under a path with special symbols don't get restored #29993
Conversation
don't get restored Fix path of restored node
if (expectedNodePathName.contains(DBNModel.SLASH_ESCAPE_TOKEN)) { | ||
expectedNodePathName = expectedNodePathName.replace(DBNModel.SLASH_ESCAPE_TOKEN, "/"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move outside the loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I believe this should be done somewhere else. Please take a look at where the path gets deserialized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe @alexander-skoblikov has concerns about this change as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, the editor "expectedName" has a node path like %2F...%2F
In other places node path convertation from %2F to /
leads to a wrong segment identification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The methods normalize/denormalize
were extracted to Utils.
don't get restored Fix path of restored node
…:dbeaver/dbeaver.git into dbeaver/pro#2795-editor-restore-path
git@github.com:dbeaver/dbeaver.git into dbeaver/pro#2795-editor-restore-path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
normailizeNodePath
-> escapeNodeId
deNormalizeNodePath
-> unescapeNodeId
What do you think?
* @return - node path object | ||
*/ | ||
public static NodePath decodeNodePath(@NotNull NodePath nodePath) { | ||
if (!nodePath.toString().contains(DBNModel.PATTERN_ESCAPE_TOKEN)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nodePath.toString() is also expensive.
Do not check NodePAth, check path
String variable.
We have another error if schema name contains |
@@ -57,7 +57,7 @@ | |||
* (e.g. TreeViewer sometimes update only first TreeItem corresponding to model certain model object). | |||
*/ | |||
public class DBNModel implements IResourceChangeListener { | |||
public static final String SLASH_ESCAPE_TOKEN = "%2F"; | |||
public static final String PATTERN_ESCAPE_TOKEN = "%"; //$NON-NLS-1$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove it.
@@ -271,6 +271,9 @@ private static NodePath getNodePath(@NotNull String path) { | |||
break; | |||
} | |||
} | |||
if (path.contains(DBNModel.PATTERN_ESCAPE_TOKEN)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used here and assumes something about how paths are escaped. Check this within the decodeNodePath
method and don't introduce new constants - it introduces unnecessary noise.
plugins/org.jkiss.dbeaver.model/src/org/jkiss/dbeaver/model/navigator/DBNDatabaseNode.java
Show resolved
Hide resolved
plugins/org.jkiss.dbeaver.model/src/org/jkiss/dbeaver/model/navigator/DBNUtils.java
Outdated
Show resolved
Hide resolved
plugins/org.jkiss.dbeaver.model/src/org/jkiss/dbeaver/model/navigator/DBNUtils.java
Outdated
Show resolved
Hide resolved
…:dbeaver/dbeaver.git into dbeaver/pro#2795-editor-restore-path
git@github.com:dbeaver/dbeaver.git into dbeaver/pro#2795-editor-restore-path
import java.net.URLDecoder; | ||
import java.net.URLEncoder; | ||
import java.nio.charset.StandardCharsets; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant imports
@@ -29,6 +29,7 @@ | |||
import org.jkiss.dbeaver.model.app.DBPProject; | |||
import org.jkiss.dbeaver.model.exec.DBCExecutionContext; | |||
import org.jkiss.dbeaver.model.exec.DBCExecutionContextDefaults; | |||
import org.jkiss.dbeaver.model.navigator.DBNModel.NodePath; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant import
NodePath nodePath = getNodePath(path); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant change
@@ -139,7 +139,8 @@ private static DBNNode legacyFindNodeByPath( | |||
|
|||
final List<String> pathItems = nodePath.pathItems; | |||
for (int i = firstItem, itemsSize = pathItems.size(); i < itemsSize; i++) { | |||
String item = pathItems.get(i).replace(DBNModel.SLASH_ESCAPE_TOKEN, "/"); | |||
//String item = pathItems.get(i).replace(DBNModel.SLASH_ESCAPE_TOKEN, "/"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented-out code
Verified |
…:dbeaver/dbeaver.git into dbeaver/pro#2795-editor-restore-path
The node path with spec symbol converted for comparison.